Skip to content

bpo-45155 : Default arguments for int.to_bytes(length=1, byteorder=sys.byteorder)#28265

Merged
warsaw merged 19 commits intopython:mainfrom
warsaw:bchr
Sep 16, 2021
Merged

bpo-45155 : Default arguments for int.to_bytes(length=1, byteorder=sys.byteorder)#28265
warsaw merged 19 commits intopython:mainfrom
warsaw:bchr

Conversation

@warsaw
Copy link
Copy Markdown
Member

@warsaw warsaw commented Sep 9, 2021

In the PEP 467 discussion, I proposed being able to use

>>> (65).to_bytes()
b'A'

IOW, adding default arguments for the length and byteorder arguments to int.to_bytes()

It occurs to me that this is (1) useful on its own merits; (2) easy to do. So I've done it.

https://bugs.python.org/issue45155

@rhettinger
Copy link
Copy Markdown
Contributor

This is a nice improvement. Thanks.

Copy link
Copy Markdown
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks reasonable to me. But .format() instead of f-strings? What?!

@rhettinger
Copy link
Copy Markdown
Contributor

The main docs need to be updated as well: https://docs.python.org/3/library/stdtypes.html?highlight=to_bytes#int.to_bytes

@warsaw
Copy link
Copy Markdown
Member Author

warsaw commented Sep 9, 2021

This all looks reasonable to me. But .format() instead of f-strings? What?!

Ha! I knew you'd say that 😆
I wanted to minimally edit the line so it would fit within my editor line length. That looked like some Python 2.6 code to me!

The main docs need to be updated as well: https://docs.python.org/3/library/stdtypes.html?highlight=to_bytes#int.to_bytes

Yep, thanks @rhettinger ... coming up!

@rhettinger
Copy link
Copy Markdown
Contributor

Also consider updating the default byteorder for int.from_bytes. The to/from round trip should be as symmetrical as possible.

Copy link
Copy Markdown
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beat me to it! I agree that int.from_bytes deserves the same treatment, too.

I also saw one possible improvement:

Comment thread Objects/longobject.c Outdated
Comment thread Doc/library/stdtypes.rst Outdated
@encukou
Copy link
Copy Markdown
Member

encukou commented Sep 10, 2021

Please, for the sake of reproducible science (and other cases), don't add platform-dependent defaults.

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Comment thread Objects/longobject.c Outdated
Comment thread Misc/NEWS.d/next/Core and Builtins/2021-09-09-15-05-17.bpo-45155.JRw9TG.rst Outdated
@bedevere-bot
Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@warsaw
Copy link
Copy Markdown
Member Author

warsaw commented Sep 10, 2021

I have made the requested changes; please review again.

@bedevere-bot
Copy link
Copy Markdown

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

Copy link
Copy Markdown
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one RST nit:

Comment thread Doc/library/stdtypes.rst Outdated
warsaw and others added 2 commits September 13, 2021 09:34
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw warsaw requested a review from brandtbucher September 15, 2021 04:50
@warsaw
Copy link
Copy Markdown
Member Author

warsaw commented Sep 15, 2021

"big" by default.

Copy link
Copy Markdown
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed some!

Comment thread Doc/library/stdtypes.rst Outdated
Comment thread Doc/library/stdtypes.rst Outdated
Comment thread Doc/library/stdtypes.rst Outdated
Comment thread Doc/library/stdtypes.rst Outdated
Comment thread Misc/NEWS.d/next/Core and Builtins/2021-09-09-15-05-17.bpo-45155.JRw9TG.rst Outdated
Comment thread Objects/longobject.c
@bedevere-bot
Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

warsaw and others added 5 commits September 15, 2021 09:41
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
…55.JRw9TG.rst

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw
Copy link
Copy Markdown
Member Author

warsaw commented Sep 15, 2021

Thanks for catching the ones I missed @brandtbucher

@warsaw
Copy link
Copy Markdown
Member Author

warsaw commented Sep 15, 2021

I have made the requested changes; please review again.

@bedevere-bot
Copy link
Copy Markdown

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

Copy link
Copy Markdown
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I promise this is my last review!

Just realized that we can help AC give us a better __text_signature__ (for help, inspect, etc.). Other than that improvement, this looks good!

Comment thread Objects/longobject.c Outdated
Comment thread Objects/longobject.c Outdated
warsaw and others added 2 commits September 15, 2021 13:28
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@brandtbucher
Copy link
Copy Markdown
Member

(I think you need to re-run AC locally, too.)

@warsaw warsaw merged commit 07e737d into python:main Sep 16, 2021
@bedevere-bot
Copy link
Copy Markdown

@warsaw: Please replace # with GH- in the commit message next time. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants